Conversation
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
nishika26
requested review from
AkhileshNegi and
avirajsingh7
and removed request for
AkhileshNegi and
avirajsingh7
nishika26
requested review from
AkhileshNegi,
Ishankoradia and
avirajsingh7
and removed request for
AkhileshNegi
| api_key = session.get(APIKey, api_key_id) | ||
|
|
||
| if not api_key or api_key.is_deleted: | ||
| raise ValueError("API key not found or already deleted") |
There was a problem hiding this comment.
we should avoid raising HTTPException in the crud layer. Use ValueError or a custom exception instead, and convert it to an HTTPException in the API route.
It’s a data layer; it shouldn't know about HTTP.
Also this is reusable method and may be used some where else in application.
@AkhileshNegi or @Ishankoradia Can confirm on this?
There was a problem hiding this comment.
I am not fussy about either.
If we do raise ValueError, we should avoid catching and re-raising in the API route, since the global exception handler (Exception) is already doing this. In such cases the status code will mostly be 500, if we do want to send a specific status code, then re-raising makes sense (or in such cases, raising HttpException also makes sense)
|
|
||
| app.include_router(api_router, prefix=settings.API_V1_STR) | ||
|
|
||
| app.add_exception_handler(HTTPException, http_exception_handler) |
There was a problem hiding this comment.
we should remove http_exception_handler completely if not needed.
Please remove this from app/api/deps.py also.
There was a problem hiding this comment.
Might seem like a redundant step. But i see 2 benefits of doing this
- A standard APi response for failures if we already dont have one. Also if we want to change this structure in the future, it becomes very easy
- In future if we integrate any observability tool, it becomes very easy to send (integrate) the error logs from this one place to the monitoring tool
What do you think ?
There was a problem hiding this comment.
Sorry but I’m not fully clear on points you mentioned.
The http_exception_handler(this function was old one do not get confused with new function) was implemented to convert all HTTP exceptions into a standardized format. However, since we are already using a global exception handler that achieves the same goal, we don’t need the http_exception_handler.
There was a problem hiding this comment.
Which part you didn't understand ?
I will leave it up to you guys to take the final call. 😄 @AkhileshNegi
| api_key = session.get(APIKey, api_key_id) | ||
|
|
||
| if not api_key or api_key.is_deleted: | ||
| raise ValueError("API key not found or already deleted") |
There was a problem hiding this comment.
I am not fussy about either.
If we do raise ValueError, we should avoid catching and re-raising in the API route, since the global exception handler (Exception) is already doing this. In such cases the status code will mostly be 500, if we do want to send a specific status code, then re-raising makes sense (or in such cases, raising HttpException also makes sense)
|
|
||
| app.include_router(api_router, prefix=settings.API_V1_STR) | ||
|
|
||
| app.add_exception_handler(HTTPException, http_exception_handler) |
There was a problem hiding this comment.
Might seem like a redundant step. But i see 2 benefits of doing this
- A standard APi response for failures if we already dont have one. Also if we want to change this structure in the future, it becomes very easy
- In future if we integrate any observability tool, it becomes very easy to send (integrate) the error logs from this one place to the monitoring tool
What do you think ?
4 participants
Summary
Target issue is #125
Explain the motivation for making this change. What existing problem does the pull request solve?
This PR introduces a centralized exception handling mechanism and integrates it across all relevant routes to ensure consistent error responses across the platform. This PR removes redundant try/except blocks across multiple routes and CRUD layers, replacing them with consistent exception propagation which is handled globally (wiith @app.exception_handler)